feat(core): readMetaRequestScope lifts _meta into RequestEnv (SEP-2575)#2063
feat(core): readMetaRequestScope lifts _meta into RequestEnv (SEP-2575)#2063felixweinberger wants to merge 5 commits into
Conversation
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
d7d52d9 to
ef0c0ce
Compare
felixweinberger
force-pushed
the
fweinberger/s4-meta-scope
branch
from
de534df to
28db3a3
Compare
|
@claude review |
There was a problem hiding this comment.
28db3a3 correctly fixes the undefined-env clobber — the remaining inline notes are non-blocking nits. Deferring to a maintainer for sign-off on the new public surface (readMetaRequestScope, MetaRequestScope, the four META_*_KEY constants, and the four new ctx.mcpReq fields).
Extended reasoning...
Overview
This PR implements the SEP-2575 per-request peer-scope mechanism in @modelcontextprotocol/core: Dispatcher.dispatch now lifts clientCapabilities / protocolVersion / clientInfo / logLevel from request.params._meta['io.modelcontextprotocol/*'] into RequestEnv, and surfaces them on ctx.mcpReq. It adds a MetaRequestScope type, a readMetaRequestScope() helper, four META_*_KEY string constants, intersects RequestEnv with MetaRequestScope, and adds two dispatcher tests. Six new symbols are added to the curated public barrel (exports/public/index.ts).
What changed since my last review
My prior 🔴 comment (explicit-undefined adapter env keys clobbering _meta-lifted values on the stateless shttp path) was addressed in 28db3a3 by stripping undefined own-keys from env before spreading over the meta scope. I re-read dispatcher.ts:155-163 and the fix is correct and well-commented; the resolved thread reflects that. The prior 🟡 nit (typeof x === 'object' admits arrays before the as Implementation / as ClientCapabilities casts) was left as-is — I flagged it as non-blocking and still consider it so. This run adds one new nit: no test exercises the { field: undefined } + _meta case, so a revert of 28db3a3 would pass CI; also non-blocking.
Security risks
Low. _meta is untrusted client input, but the lifted values are advisory ("client-asserted" per the doc comment) and only widen what the server may attempt toward the client — they don't grant the client anything. Worst case for the unaddressed array-guard nit is a handler TypeError on malformed input, which _dispatchCore already converts to a JSON-RPC InternalError. No auth, crypto, or permission logic is touched.
Level of scrutiny
Medium-high. The runtime change is small and now correct, but per REVIEW.md ("Every new export is intentional", "Burden of proof is on addition") the six new public exports and the four new fields on BaseContext.mcpReq are an API-surface decision a maintainer should ratify — e.g., whether readMetaRequestScope belongs in the public barrel or stays internal (the dispatcher already calls it on every request, so userland rarely needs it directly), and whether the META_* key strings are spec-stable enough to export. This is S4 in a stacked SEP-2575 series with a downstream consumer (F1), so the shape here propagates.
Other factors
Tests pass (1364 + 101 skipped; dual conformance 40/40 per the description). No changeset yet, which is normal for stacked work. The two outstanding inline items are both nits; neither blocks merge on correctness grounds.
| test('adapter-supplied env wins over _meta', async () => { | ||
| const d = new Dispatcher(); | ||
| let seenPv: string | undefined; | ||
| d.setRequestHandler('ping', async (_r, ctx) => { | ||
| seenPv = ctx.mcpReq.protocolVersion; | ||
| return {}; | ||
| }); | ||
| await collect( | ||
| d.dispatch(req('ping', { _meta: { 'io.modelcontextprotocol/protocolVersion': 'from-meta' } }), { | ||
| protocolVersion: 'from-env' | ||
| }) | ||
| ); | ||
| expect(seenPv).toBe('from-env'); | ||
| }); |
There was a problem hiding this comment.
🟡 nit: commit 28db3a3 fixed the explicit-undefined env clobber (dispatcher.ts:161-163), but neither test in this block exercises it — one passes no env, the other passes a defined protocolVersion. Reverting to plain { ...readMetaRequestScope(...), ...env } would still pass the suite, so worth a third case (env: { clientCapabilities: undefined } + _meta present) to lock in the fix for the shttpHandler stateless-POST path.
Extended reasoning...
What this is
This is a test-coverage gap, not a runtime bug. The previous review round flagged that { ...readMetaRequestScope(...), ...env } let an adapter env with explicit undefined keys clobber the _meta-lifted values (and that shttpHandler.ts:331-337 does exactly this on a stateless POST). Commit 28db3a3 fixed the production code by stripping undefined own-keys from env before spreading:
const definedEnv: RequestEnv = {};
for (const [k, v] of Object.entries(env)) if (v !== undefined) (definedEnv as Record<string, unknown>)[k] = v;
const enrichedEnv: RequestEnv = { ...readMetaRequestScope(request.params?._meta), ...definedEnv };That fix is correct. However, the resolved review comment also said "Worth adding a test for env: { clientCapabilities: undefined } + _meta present", and no such test was added.
Why the existing tests don't cover it
The two new tests in the MetaRequestScope (SEP-2575) block are:
- "clientCapabilities/protocolVersion lifted from _meta onto ctx.mcpReq" — calls
d.dispatch(req('ping', { _meta: {...} }))with no second argument, soenvdefaults to{}.Object.entries({})is empty; the strip-undefined loop runs zero iterations. - "adapter-supplied env wins over _meta" — passes
{ protocolVersion: 'from-env' }. The value is defined, so it would win under either the old plain-spread or the new defined-only merge.
Neither passes an env literal with an explicit field: undefined while _meta supplies the same field. A grep across packages/**/*.test.ts for clientCapabilities:\s*undefined|protocolVersion:\s*undefined returns no hits.
Step-by-step proof that the suite would pass on a regression
- Revert dispatcher.ts:161-163 to
const enrichedEnv: RequestEnv = { ...readMetaRequestScope(request.params?._meta), ...env };. - Run test 1:
envis{};{ ...{caps, pv}, ...{} }→{caps, pv}. Passes. - Run test 2:
envis{ protocolVersion: 'from-env' };{ ...{pv:'from-meta'}, ...{pv:'from-env'} }→{pv:'from-env'}. Passes. - Yet on the real shttpHandler path,
baseEnvwould again containclientCapabilities: undefinedas an own property, and{ ...{clientCapabilities:{sampling:{}}}, ...{clientCapabilities:undefined} }→{clientCapabilities:undefined}— the SEP-2575 mechanism silently re-broken with green CI.
Why it matters
The defined-only merge exists specifically to guard the primary SEP-2575 stateless-HTTP path (the inline comment at dispatcher.ts:158 names shttpHandler explicitly). { field: maybeUndefined } is idiomatic TS, so a future refactor of either the dispatcher or any adapter could easily reintroduce the bug. REVIEW.md → Tests & docs: "New behavior has vitest coverage including error paths."
Suggested fix
Add a third test alongside the existing two:
test('env with explicit-undefined key does not clobber _meta', async () => {
const d = new Dispatcher();
let seenCaps: unknown;
d.setRequestHandler('ping', async (_r, ctx) => {
seenCaps = ctx.mcpReq.clientCapabilities;
return {};
});
await collect(
d.dispatch(
req('ping', { _meta: { 'io.modelcontextprotocol/clientCapabilities': { sampling: {} } } }),
{ clientCapabilities: undefined } // shttpHandler shape on stateless POST
)
);
expect(seenCaps).toEqual({ sampling: {} });
});Non-blocking — the implementation is correct; this just locks it in.
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
ef0c0ce to
c1b00ac
Compare
felixweinberger
force-pushed
the
fweinberger/s4-meta-scope
branch
from
28db3a3 to
6c2c486
Compare
|
@claude review |
…ocolVersion,logLevel} into RequestEnv (SEP-2575)
… keys do not clobber _meta-lifted scope
…nfo via schema (parity with initialize)
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
c1b00ac to
e579bff
Compare
felixweinberger
force-pushed
the
fweinberger/s4-meta-scope
branch
from
3d3f254 to
740466a
Compare
|
@claude review |
There was a problem hiding this comment.
Both earlier issues are fixed and the implementation looks correct to me — the remaining inline note is a non-blocking test-coverage suggestion you can take or leave. I'm not auto-approving only because this adds public API surface (readMetaRequestScope, MetaRequestScope, the four META_*_KEY constants, and new ctx.mcpReq fields) for a SEP, which is the kind of design surface a maintainer should sign off on; a changeset is also still missing.
Extended reasoning...
Overview
This PR implements the per-request peer-scope mechanism of SEP-2575: Dispatcher.dispatch() now lifts clientCapabilities / clientInfo / protocolVersion / logLevel from request.params._meta['io.modelcontextprotocol/*'] into RequestEnv, merges them under any defined adapter-supplied env values, and surfaces them on ctx.mcpReq. It touches packages/core/src/shared/context.ts (new MetaRequestScope type + readMetaRequestScope() helper + four new mcpReq fields), dispatcher.ts (the merge in the dispatch() hot path), types/constants.ts (four new META_*_KEY constants), exports/public/index.ts (six new public exports), and adds two dispatcher tests.
Review history on this PR
- Round 1 flagged a 🔴 bug where
{ ...readMetaRequestScope(...), ...env }let an adapter env with explicitundefinedkeys clobber the_meta-lifted values on the stateless-HTTP path, plus a 🟡 nit thattypeof x === 'object'admitted arrays beforeas Implementation/as ClientCapabilities. Both fixed: the dispatcher now strips undefined own-keys fromenvbefore spreading, andreadMetaRequestScoperunsclientInfo/clientCapabilitiesthroughImplementationSchema/ClientCapabilitiesSchema.safeParse(which also gives parity with theinitializepath'sz.preprocessnormalisations). - Round 2 left a 🟡 non-blocking suggestion to add a third test pinning the explicit-
undefinedmerge behaviour. That's still open but was explicitly marked non-blocking; the implementation is correct without it. - Since round 2 the only changes are 2a91eaa (drop
@linkcodereferences to the privateDispatcher.dispatch/RequestEnvfrom the publicMetaRequestScopeJSDoc) and 740466a (prettier). No behaviour change.
Security risks
Low. _meta is client-controlled wire input, but the new code now schema-validates the structured fields via safeParse and the values are advisory/client-asserted (capability checks fail closed when absent or malformed). No auth, crypto, or filesystem surface is touched.
Level of scrutiny
Moderate-to-high. The change is small and mechanically clean, but it (a) modifies the core dispatch() hot path that every request flows through, (b) adds six new public exports plus four new fields on BaseContext.mcpReq, and (c) implements a protocol SEP. Per this repo's conventions ("burden of proof is on addition", "every new export is intentional"), public-API growth tied to a SEP is exactly the category a human maintainer should sign off on rather than a bot — e.g., whether readMetaRequestScope belongs in the public surface or should stay internal until F1 lands, and whether the META_*_KEY strings match the SEP text verbatim.
Other factors
- Changeset: still missing (changeset-bot warning is live). Since this adds public exports, a minor-bump changeset is presumably wanted before merge.
- Tests: two new dispatcher tests cover the lift and the env-wins precedence; the defined-only merge is exercised indirectly but not pinned by a dedicated case (open non-blocking nit).
- Docs: JSDoc is thorough; no prose docs/examples added, which seems fine for an infrastructure piece consumed by the follow-up F1 PR.
- Stacked: PR description notes this depends on S3 and is consumed by F1, so a maintainer reviewing the stack as a whole is the right audience.
1 participant
dispatch()liftsclientCapabilities/protocolVersion/logLevelfromrequest.params._meta['io.modelcontextprotocol/*']intoRequestEnvper call. AddsMetaRequestScopetype andreadMetaRequestScope()helper.Motivation and Context
Implements the per-request peer-scope mechanism of SEP-2575 (stateless protocol) — a stateless server gets per-request capabilities without an
initializehandshake.How Has This Been Tested?
New dispatcher tests; 1364 tests + 101 skipped; dual conformance both 40/40.
Breaking Changes
None.
Types of changes
Checklist
Additional context
Depends on S3. F1 consumes this. Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.